-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
procstat: Add a 'compartments' command to list c18n compartments #2276
base: dev
Are you sure you want to change the base?
Conversation
struct cheri_c18n_compart { | ||
ssize_t ccc_id; | ||
char ccc_name[CHERI_C18N_COMPART_MAXNAME]; | ||
char _ccc_pad[64]; /* Shrink as new fields added above. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should preemptively define a 'ccc_flags' field to capture concepts like "This is a non-default sub-library compartment", "this compartment can performance system calls", and similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can always add them in the future when they become needed. Right now RTLD doesn't track such information about compartments, so it is risky to to add the flags prematurely.
*/ | ||
if (len != sizeof(info) || | ||
info.version != CHERI_C18N_INFO_VERSION || | ||
info.comparts_gen % 2 != 0 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somewhat wondered if we wanted some sort of memory-barrier arrangement to ensure that we got a clean(ish) snapshot -- i.e., that if we saw the current generation, we saw all the stores we read from the compartment / string tables came before the generation number we read was stored, and that at the end of the sysctl function we haven't seen any stores that post-dated that generation-number store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every iteration below we also re-read the generation number and check that it hasn’t changed. Presumably this achieves the desired effect?
return; | ||
} | ||
if ((procstat_opts & PS_OPT_NOHEADER) == 0) | ||
xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always feel that 19 characters wide is quite a long slot for COMM, which most of the time uses list. Not sure if other procstat/ps modes might give less by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 19 is fine (in fact it is too short for the cheribsdtest
variants). And the existing c18n
and cheri
commands both use 19.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I widened to 19 (MAXCOMLEN) for auxv in 64e9f6a. It can be shorter, but it should probably the right most column so it can safely spill if it's not going to be MAXCOMLEN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compartment name needs to be last and is likely to be the very long thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking it would be nice if this were split into kernel, libprocstat, and prostatic commits.
@@ -47,6 +53,8 @@ | |||
.Nm procstat_getargv , | |||
.Nm procstat_getauxv , | |||
.Nm procstat_getenvv , | |||
.Nm procstat_getc18n , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting this is good, but it should be a separate commit.
return; | ||
} | ||
if ((procstat_opts & PS_OPT_NOHEADER) == 0) | ||
xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I widened to 19 (MAXCOMLEN) for auxv in 64e9f6a. It can be shorter, but it should probably the right most column so it can safely spill if it's not going to be MAXCOMLEN.
44db10c
to
acd2d25
Compare
acd2d25
to
dd8c97b
Compare
* The interface provided by the kernel via sysctl for compartmentalization | ||
* monitoring tools such as procstat. | ||
*/ | ||
#define CHERI_C18N_COMPART_MAXNAME 56 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry this should be more like PATH_MAX + NAME_MAX + 2 (separator and terminator).
#define CHERI_C18N_COMPART_MAXNAME 56 | ||
#define CHERI_C18N_COMPART_LAST -1 | ||
|
||
struct cheri_c18n_compart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I would model this on other kinfo_* structures. I would embed a size field at the start and move the name field to the end. The name could be packed in the exported form similar to what we do for kinfo_file
and kinfo_vmobject
sysctls now. Userspace would unpack it back out into a simple array.
@@ -2534,8 +2534,8 @@ sysctl_kern_proc_c18n(SYSCTL_HANDLER_ARGS) | |||
info.version != CHERI_C18N_INFO_VERSION || | |||
info.stats_size == 0 || | |||
info.stats_size > RTLD_C18N_STATS_MAX_SIZE || | |||
!__CAP_CHECK(info.stats, info.stats_size) || | |||
(cheri_getperm(info.stats) & CHERI_PERM_LOAD) == 0) { | |||
!cheri_can_access(info.stats, CHERI_PERM_LOAD, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two hunks seem to be unrelated and some other bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just replacing the __CAP_CHECK
pattern with cheri_can_access
to resemble what sysctl_kern_proc_c18n_compartments
is doing below.
if (!cheri_can_access(sptr, CHERI_PERM_LOAD, | ||
(__cheri_addr ptraddr_t)&sptr[n], 1)) | ||
return (-1); | ||
readlen = proc_readmem(td, p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can likely do a bit better than reading one byte at a time. That is, you can fetch the remaining bytes for the current page up to the limit of len
or the remaining length of sptr
. You could always read it into a temporary PAGE_SIZE'd buffer and only copy out to buf
up to the first \0
. That will be significantly more efficient.
@@ -1065,6 +1065,7 @@ TAILQ_HEAD(sysctl_ctx_list, sysctl_ctx_entry); | |||
#define KERN_PROC_REVOKER_STATE 47 /* revoker state */ | |||
#define KERN_PROC_REVOKER_EPOCH 48 /* revoker epoch */ | |||
#define KERN_PROC_C18N 49 /* compartmentalisation statistics */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it may be worth renaming this to KERN_PROC_C18N_STATS
error = ENOEXEC; | ||
goto out; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You really want to optimize the first call to sysctl() with a NULL oldptr from userspace that is just trying to query the size to use. You can do a decent estimate by just reading in the top-level structure with the count of compartments and using that to compute the size needed and use that with SYSCTL_OUT with a NULL input buffer and then return early without reading the actual compartment list from userspace.
@@ -399,6 +405,53 @@ procstat_getc18n(struct procstat *procstat, struct kinfo_proc *kp, | |||
return (-1); | |||
} | |||
|
|||
int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not require the caller to allocate an array and hope for the best. :) It should return an allocated array of objects that the caller can free. You can then call the sysctl twice, once to get the size estimate and a second time to populate it. This requires fixing the sysctl to add the optimized path for querying the size I mentioned.
|
||
#include "procstat.h" | ||
|
||
#define C18N_MAX_COMPARTS 1024 /* Horrible but functional, for now. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this hack can go away as it should.
return; | ||
} | ||
if ((procstat_opts & PS_OPT_NOHEADER) == 0) | ||
xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compartment name needs to be last and is likely to be the very long thing.
@bsdjhb Would be curious to hear your thoughts about the generation counter as a fix for the race condition. Does this seem sound to you? |
Yes, that pattern is used elsewhere for the same trick. |
This rebases #2272 to dev and adds more features (incl. a generation counter for the compartment array to deal with races).